contracts/test/foundry/KlerosCore_Appeals.t.sol (4)

609-619: Pin event emitters to avoid false positives

Tie expected logs to their emitters to ensure they come from the intended contracts.

Apply this diff:

-        vm.expectEmit(true, true, true, true);
+        vm.expectEmit(address(core));
         emit KlerosCore.CourtJump(disputeID, 1, courtID3, courtID2);
-        vm.expectEmit(true, true, true, true);
+        vm.expectEmit(address(core));
         emit KlerosCore.DisputeKitJump(disputeID, 1, dkID3, dkID2);
-        vm.expectEmit(true, true, true, true);
+        vm.expectEmit(address(disputeKit2));
         emit DisputeKitClassicBase.DisputeCreation(disputeID, 2, newExtraData);
-        vm.expectEmit(true, true, true, true);
+        vm.expectEmit(address(core));
         emit KlerosCore.AppealDecision(disputeID, arbitrable);
-        vm.expectEmit(true, true, true, true);
+        vm.expectEmit(address(core));
         emit KlerosCore.NewPeriod(disputeID, KlerosCore.Period.evidence);

---

685-691: Also pin emitters on the second jump (and consider asserting NewPeriod)

Use specific emitters here as well; optionally assert NewPeriod(evidence) like in the first jump.

-        vm.expectEmit(true, true, true, true);
+        vm.expectEmit(address(core));
         emit KlerosCore.CourtJump(disputeID, 2, courtID2, GENERAL_COURT);
-        vm.expectEmit(true, true, true, true);
+        vm.expectEmit(address(core));
         emit KlerosCore.DisputeKitJump(disputeID, 2, dkID2, dkID3);
+        vm.expectEmit(address(core));
+        emit KlerosCore.NewPeriod(disputeID, KlerosCore.Period.evidence);

---

605-606: Avoid magic funding amounts; compute from appealCost and multipliers

Hard-coded values (0.63, 0.42, 1.35, 0.9 ether) will drift if fees/multipliers change. Derive from core.appealCost(disputeID).

Example:

-        disputeKit3.fundAppeal{value: 0.63 ether}(disputeID, 1);
+        uint256 ac1 = core.appealCost(disputeID);
+        uint256 lose1 = ac1 + (ac1 * 20000) / Constants.MULTIPLIER_DIVISOR;
+        disputeKit3.fundAppeal{value: lose1}(disputeID, 1);
...
-        disputeKit3.fundAppeal{value: 0.42 ether}(disputeID, 2);
+        uint256 win1 = ac1 + (ac1 * 10000) / Constants.MULTIPLIER_DIVISOR;
+        disputeKit3.fundAppeal{value: win1}(disputeID, 2);
...
-        disputeKit2.fundAppeal{value: 1.35 ether}(disputeID, 1);
+        uint256 ac2 = core.appealCost(disputeID);
+        uint256 lose2 = ac2 + (ac2 * 20000) / Constants.MULTIPLIER_DIVISOR;
+        disputeKit2.fundAppeal{value: lose2}(disputeID, 1);
...
-        disputeKit2.fundAppeal{value: 0.9 ether}(disputeID, 2);
+        uint256 win2 = ac2 + (ac2 * 10000) / Constants.MULTIPLIER_DIVISOR;
+        disputeKit2.fundAppeal{value: win2}(disputeID, 2);

Note: If multipliers are accessible via getters, prefer them over literals.

Also applies to: 620-621, 683-684, 690-691

---

758-766: Optional: assert total rounds after the second jump

Quick sanity on core-side tracking.

         core.executeRuling(disputeID); // winning choice is 2
+        assertEq(core.getNumberOfRounds(disputeID), 3, "Should have 3 rounds after two appeals");

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)

209-221: Recurring DK reuse path looks good; consider idempotent active-flag set

Reusing the existing localDisputeID and resetting jumped=false is correct for returning disputes. Optionally reassert coreDisputeIDToActive[_coreDisputeID] = true; in both branches to be explicit/idempotent.

         if (coreDisputeIDToActive[_coreDisputeID]) {
             localDisputeID = coreDisputeIDToLocal[_coreDisputeID];
             dispute = disputes[localDisputeID];
             dispute.jumped = false;
+            coreDisputeIDToActive[_coreDisputeID] = true; // idempotent
         } else {
             localDisputeID = disputes.length;
             dispute = disputes.push();
             coreDisputeIDToLocal[_coreDisputeID] = localDisputeID;
             coreDisputeIDToActive[_coreDisputeID] = true;
         }

---

229-231: Guard against accidental underflow and cache the round ID

Minor: cache the core round ID and assert it exists before subtracting. Improves readability and prevents a panic if call ordering ever changes.

-        // New round in the Core should be created before the dispute creation in DK.
-        dispute.coreRoundIDToLocal[core.getNumberOfRounds(_coreDisputeID) - 1] = dispute.rounds.length;
+        // New round in the Core should be created before the dispute creation in DK.
+        uint256 coreRoundID = core.getNumberOfRounds(_coreDisputeID);
+        require(coreRoundID > 0, "Core round not initialized");
+        dispute.coreRoundIDToLocal[coreRoundID - 1] = dispute.rounds.length; // 1-based storage

---

462-463: Avoid -1 underflow panics; centralize local round lookup

All these sites assume coreRoundIDToLocal[_coreRoundID] != 0 and do - 1. If unset, you get a panic (arithmetic or OOB). Tests currently rely on panics to detect “wrong DK/round”, but a targeted error improves debuggability and future flexibility.

Suggestion:

• Add an internal helper returning the 0-based localRound index with a descriptive revert.
• Use it across read paths to remove duplication and accidental off-by-one errors.

// Add once:
error RoundNotHandledByThisDK();
function _localRoundIndexOrRevert(Dispute storage d, uint256 _coreRoundID) internal view returns (uint256) {
    uint256 idx1 = d.coreRoundIDToLocal[_coreRoundID];
    if (idx1 == 0) revert RoundNotHandledByThisDK(); // not mapped in this DK
    return idx1 - 1; // convert to 0-based
}

Then replace patterns like:

Round storage round = dispute.rounds[dispute.coreRoundIDToLocal[_coreRoundID] - 1];
Vote storage vote = dispute.rounds[dispute.coreRoundIDToLocal[_coreRoundID] - 1].votes[_voteID];
localRoundID = disputes[localDisputeID].coreRoundIDToLocal[_coreRoundID] - 1;

with:

uint256 lr = _localRoundIndexOrRevert(dispute, _coreRoundID);
Round storage round = dispute.rounds[lr];
Vote storage vote = dispute.rounds[lr].votes[_voteID];
localRoundID = _localRoundIndexOrRevert(disputes[localDisputeID], _coreRoundID);

Note: Keeping current tests is possible by also testing for RoundNotHandledByThisDK() in addition to panics.

Also applies to: 566-567, 579-580, 647-648, 670-671, 698-699, 708-709

contracts/test/foundry/KlerosCore_Appeals.t.sol (1)

471-800: Strong E2E coverage for recurring DK; consider reducing brittleness and duplication

• Reliance on stdError.arithmeticError is brittle; a future switch to explicit custom errors would break tests. Consider accepting both via try/catch or dedicated helpers.
• Extract small helpers for repeated steps (stake/draw/vote/fund/expectJump) to cut boilerplate and improve readability.

If you want, I can propose a small set of reusable helpers for these flows.

Please confirm if we should keep asserting for panics vs. migrate to explicit error types in DK.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

contracts/src/arbitration/interfaces/IDisputeKit.sol (1)

33-45: Clarify the precondition and call ordering for createDispute.

Good note. Recommend tightening the NatSpec to state the ordering and expected behavior on violation.

     /// @notice Creates a local dispute and maps it to the dispute ID in the Core contract.
     /// @dev Access restricted to Kleros Core only.
-    /// @dev The new `KlerosCore.Round` must be created before calling this function.
+    /// @dev Requirements:
+    /// - KlerosCore MUST have created the next Round for `_coreDisputeID` before calling this function.
+    /// - Implementations SHOULD revert if the round does not exist or the dispute is not active in this DK.
+    /// - Implementations SHOULD reuse an existing local dispute if already active (recurring DK scenarios).
     /// @param _coreDisputeID The ID of the dispute in Kleros Core, not in the Dispute Kit.

contracts/test/foundry/KlerosCore_Disputes.t.sol (1)

95-98: Minor: use disputeID variable and clearer tuple names.

Improves readability and avoids hardcoded IDs.

-        (bool dispute, bool currentRound) = disputeKit.coreDisputeIDToActive(0);
-        assertEq(dispute, true, "Dispute should be active in this DK");
-        assertEq(currentRound, true, "Current round should be active in this DK");
+        (bool disputeActive, bool currentRoundActive) = disputeKit.coreDisputeIDToActive(disputeID);
+        assertEq(disputeActive, true, "Dispute should be active in this DK");
+        assertEq(currentRoundActive, true, "Current round should be active in this DK");

contracts/test/foundry/KlerosCore_Execution.t.sol (1)

724-724: LGTM: withdrawal API and event updates reflected. Add idempotency check (optional).

The calls and event args match the simplified signature. Optionally, assert a second withdrawal reverts or returns 0 to cover idempotency.

Example addition after the successful withdraw:

vm.expectRevert(DisputeKitClassicBase.NothingToWithdraw.selector);
disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 1);

Also applies to: 732-732, 740-746

contracts/test/foundry/KlerosCore_Appeals.t.sol (2)

301-317: Robustness: also assert Active.dispute when checking currentRound

You only assert currentRound; add a quick check for disputeActive to ensure the DK recognizes the core dispute at both old/new DKs.

-        (, bool currentRound) = newDisputeKit.coreDisputeIDToActive(disputeID);
-        assertEq(currentRound, false, "round should be jumped");
+        (bool disputeActive, bool currentRound) = newDisputeKit.coreDisputeIDToActive(disputeID);
+        assertEq(disputeActive, true, "dispute should exist in the previous DK");
+        assertEq(currentRound, false, "round should be jumped");

---

439-455: Same nit: assert both fields of Active

Mirrors the previous suggestion for consistency and clearer failures.

-        (, bool currentRound) = disputeKit3.coreDisputeIDToActive(disputeID);
-        assertEq(currentRound, false, "round should be jumped");
+        (bool disputeActive, bool currentRound) = disputeKit3.coreDisputeIDToActive(disputeID);
+        assertEq(disputeActive, true, "dispute should exist in the previous DK");
+        assertEq(currentRound, false, "round should be jumped");

-        (, currentRound) = disputeKit2.coreDisputeIDToActive(disputeID);
-        assertEq(currentRound, true, "round should be active in the DK that dispute jumped to");
+        (disputeActive, currentRound) = disputeKit2.coreDisputeIDToActive(disputeID);
+        assertEq(disputeActive, true, "dispute should exist in the new DK");
+        assertEq(currentRound, true, "round should be active in the DK that dispute jumped to");

contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (4)

111-117: Event signature change: confirm downstream consumers

Withdrawal now omits coreRoundID. Ensure off-chain indexers/UI consuming the old 4-topic signature are updated, or emit a deprecated event for one release to ease transition.

Would you like a thin compatibility event emitted alongside the new one for a deprecation window?

---

138-142: Modifier name vs semantics

isActive() now encodes both “known dispute” and “current round on this DK.” Clear. Consider renaming to requireCurrentRound() for readability, keeping isActive as a pure “known” check for public views.

---

372-436: Appeal/jump flow is correct; minor gas and clarity tweaks

• External call this.currentRuling is more expensive than reading state; if not required for polymorphism, prefer internal helper.
• When marking a jump, only currentRound is flipped; dispute remains true—good.

-        (uint256 ruling, , ) = this.currentRuling(_coreDisputeID);
+        (uint256 ruling, , ) = currentRuling(_coreDisputeID);

Make currentRuling internal/virtual (and add a public wrapper) if polymorphism is needed.

---

588-606: View functions can underflow on unknown disputes

getFundedChoices/areVotesAllCast and similar read disputes[coreDisputeIDToLocal[_coreDisputeID]] and access rounds[length-1]. If the dispute is unknown to this DK, length==0 underflows. Either:

• Add a lightweight known-dispute check (a.dispute) and return safe defaults, or
• Document that callers must use coreDisputeIDToActive first.

function getFundedChoices(uint256 _coreDisputeID) public view returns (uint256[] memory fundedChoices) {
-    Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]];
+    if (!coreDisputeIDToActive[_coreDisputeID].dispute) revert DisputeUnknownInThisDisputeKit();
+    Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]];
    ...
}

Apply similarly to other views that index the last round. As per coding guidelines

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: jaybuidl
PR: kleros/kleros-v2#2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.